Calling find on a String, this will fail. The comment used element.closest that is missing here

Yes, sorry may fault, I made a mistake while copy&pasting. Downloaded the repo locally now and made sure that test are passing in next commit. Also I think e.g. validateSimpleFormBootstrap4.js test for horizontal_form SimpleForm's bootstrap wrapper isn't absolutely correct, because current SimpleForm's bootstrap configuration would make HTML for field with error like this:

<div class="form-group row integer required form-group-invalid">
    <label class="col-sm-3 col-form-label" for="xxx">
      Label
    </label>
    <div class="col-sm-9">
      <input class="form-control is-invalid" name="xxx" id="xxx">
      <div class="invalid-feedback">Error feedback...</div>
      <small class="form-text text-muted">Input hint..</small>
    </div>
</div>

So the form-group-invalid class isn't on parent of the input but on the wrapper and that's what I am trying to address with my PR.

Thanks!

Feel free to fix the test

Anyway, In this case, I think that the error should be added to the parent element. Maybe we can add a check on the form's css class and provide two different approaches

Ok I will fix the test but then the current element.parent() method of adding wrapper_error_class would be failing with :horizontal_form wrapper. I just thought new element.closest('.form-group') method is more universal for current simple_form's bootstrap4 configuration working with most of it's wrappers.

I don't think there's wrapper's class on form's css, because wrapper is a simple_form's thing, not bootstrap's. If different approaches are needed (and I think it will be for more complex wrappers), CSV has a nice way to do this by implementing wrapper's specific add/remove methods in ClientSideValidations.formBuilders['SimpleForm::FormBuilder'] e.g.:

wrappers: {
  horizontal_form: {
    add (element, settings, message) {
      ...
    },
    remove...
      ...
  default:
    ...  
}

Ok, I would go for the closest form group, but it will not work for any use case.

Probably I went for element.parent instead of the closest wrapper that I've suggested because at the time I wrote this solution it was the choice that worked with most of the combinations

Ok I can see what you mean. The error message (.invalid-feedback) should be added to parent element, in that case element.parent() is right. But when adding wrapper_error_class, wrapper tag isn't always the parent. Maybe we should split this.. add error to parentElement and add wrapper_error_class to wrapperElement.

I will fix/add tests for horizontal_wrapper, vertical_wrappers and inline_wrapper according to current simple_form default bootstrap config and we may test it against those, what do you think?

Ok I can see what you mean. The error message (.invalid-feedback) should be added to parent element, in that case element.parent() is right. But when adding wrapper_error_class, wrapper tag isn't always the parent. Maybe we should split this.. add error to parentElement and add wrapper_error_class to wrapperElement.

Yes, it is mostly correct

wrapper_errror_class should not be needed at all by Bootstrap 4 itself, we could remove that

I can see that all examples provided here: https://getbootstrap.com/docs/4.4/components/forms/ (search for invalid-feedback) are attached to the parent element (actually the sibling of the input element, but it should not be the case of input-group-append).

I will fix/add tests for horizontal_wrapper, vertical_wrappers and inline_wrapper according to current simple_form default bootstrap config and we may test it against those, what do you think?

If you have a comprehensive page with a lot of form elements generated by the latest default bootstrap+simple form, please share the html here (or in a gist), so I can also take a look

You can find latest simple_form examples at http://simple-form-bootstrap.plataformatec.com.br/ (submit forms for invalid markup). You are right most of wrappers add invalid-feedback as sibling of the input, except for Input Group Form (but yes, bootstrap4 docs has it as sibling so maybe it works both ways. This also cause double invalid-feedback when (not) re-using existing server error block.) and special date/radio/etc.. inputs which CSV doesn't support anyway.

Anyway purpose of my PR was to fix wrapper_errror_class assignment which ATM doesn't work for Horizontal Form and Input Group Form examples. You are right wrapper_errror_class is not needed by bootstrap but it's SimpleForm's thing and people may expect it there and could use it e.g. for styling of label. Sure it's possible to customize JS formBuilder but it would be nice if it works by default.

I committed JS tests for :horizontal_form wrapper and made them pass with a change which I believe also solves double invalid-feedback for input-group fields.